Skip to content

perf(queue): claim the actuation lock before the refresh it exists to prevent - #10181

Merged
JSONbored merged 1 commit into
mainfrom
perf/claim-actuation-lock-before-refresh
Jul 31, 2026
Merged

perf(queue): claim the actuation lock before the refresh it exists to prevent#10181
JSONbored merged 1 commit into
mainfrom
perf/claim-actuation-lock-before-refresh

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Closes #10174

The publish-and-maintain pass refreshed PR details and then asked "does another pass already own this PR?" — so every contended pass did the work and threw it away.

// before
if (otherRefreshReasons || reviewsCacheStale) await refreshPullRequestDetails()   // GitHub reads
const actuationLock = await claimPrActuationLock()
if (!actuationLock.acquired) { …audit…; throw ContendedError }                     // all discarded

// after
const actuationLock = await claimPrActuationLock()
if (!actuationLock.acquired) { …audit…; throw ContendedError }                     // bail before spending
if (otherRefreshReasons || reviewsCacheStale) await refreshPullRequestDetails()

Volume

github_app.pr_public_surface_lock_contended is the most frequent audit event on the Orb — 1,180 between 09:00 and 10:53 today, ~10/min, about 2× the next event:

 github_app.pr_public_surface_lock_contended   1180
 github_app.review_deferred_ci_pending          590
 github_app.miner_detection_cache_hit           529

Cost, stated precisely

refreshPullRequestDetails is itself cached — it consults the detail-sync state and reuses stored pull_request_files rows when the last sync covered the current head SHA. So a contention does not always cost a GitHub call, and I am not claiming 1,180 wasted API calls.

It always costs the sync-state reads plus shouldRefreshFilesForPreMergeChecks; on a cache miss it costs a token fetch plus the files/reviews fetch. That miss is what a busy PR produces — and a busy PR is also what contends, so the two peak together. This is the same window in which the installation hit API rate limit exceeded for installation ID 143010787 and 66 queue jobs stalled behind deferred_by: rate_limit.

Why this is safe

No semantic change. The lock's stated purpose (#9013) is to make "does another pass already own this PR" one question with one answer for the whole publish-then-maintain unit; asking it before the expensive part is strictly better.

Holding it across the refresh is already handled — #9467 renews the lock while work runs, precisely because this unit can span an AI review far longer than a refresh.

The second contention site (~7766) is deliberately untouched: it does not refresh beforehand, so it does not have this defect.

On the test asserting source order

Unusual, and deliberate. Both orderings behave identically on the happy path and differ only in what a losing pass spends before it throws — so no behavioural test can distinguish them. That is exactly why this drifted unnoticed.

Mitigations against the usual failure modes of such a test:

  • anchored on the publish pass's own contention audit event, so an unrelated claimPrActuationLock elsewhere in this 16k-line file cannot satisfy it;
  • a sanity assertion that both landmarks still exist, so a rename cannot make it pass vacuously;
  • mutation-tested — reverting to the old ordering fails it.

Verification

Full suite 26,344 passed / 0 failed; typecheck, dead-exports, dead-source-files clean.

… prevent

The publish-and-maintain pass refreshed PR details and THEN asked "does another
pass already own this PR", so every contended pass did the work and threw it
away.

github_app.pr_public_surface_lock_contended is the single most frequent audit
event on the production Orb: 1,180 occurrences between 09:00 and 10:53 today,
roughly 10 per minute and about twice the next event. Each one is a discarded
pass.

The cost is real but worth stating precisely. refreshPullRequestDetails is
itself cached -- it consults the detail-sync state and reuses stored
pull_request_files rows when the last sync covered the current head SHA -- so a
contention does not always cost a GitHub call. It always costs the sync-state
reads, and on a cache miss it costs a token fetch plus the files/reviews fetch.
That miss is what a busy PR produces, and a busy PR is also what contends, so
the two peak together. This happened in the same window the installation
exhausted its REST quota and 66 queue jobs stalled behind deferred_by:
rate_limit.

Claiming first changes no semantics: the lock's stated purpose (#9013) is to
make "does another pass already own this PR" one question with one answer for
the whole publish-then-maintain unit, and asking it before the expensive part is
strictly better. Holding it across the refresh is already safe -- #9467 renews
the lock while work runs precisely because this unit can span an AI review far
longer than a refresh.

The second contention site is deliberately untouched: it does not refresh
beforehand, so it does not have this defect.

Guarded by a test asserting SOURCE ORDER, which is unusual and deliberate. Both
orderings behave identically on the happy path and differ only in what a LOSING
pass spends before it throws, so no behavioural test can distinguish them --
which is why this drifted unnoticed. The test anchors on the publish pass's own
contention audit event so an unrelated claim elsewhere in this 16k-line file
cannot satisfy it, and asserts both landmarks still exist so a rename cannot
make it pass vacuously.

Closes #10174
@loopover-orb

loopover-orb Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Important

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏳ LoopOver is waiting…

LoopOver has seen this pull request and is waiting on CI checks to finish before reviewing it. This comment will update once the review runs.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed · 🟨 Waiting

@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@JSONbored JSONbored self-assigned this Jul 31, 2026
@JSONbored
JSONbored merged commit db3fe36 into main Jul 31, 2026
4 checks passed
@JSONbored
JSONbored deleted the perf/claim-actuation-lock-before-refresh branch July 31, 2026 11:24
@codecov

codecov Bot commented Jul 31, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.34%. Comparing base (c1484f5) to head (c0e00f2).
⚠️ Report is 8 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10181      +/-   ##
==========================================
- Coverage   92.21%   91.34%   -0.88%     
==========================================
  Files         932      932              
  Lines      114115   114115              
  Branches    27566    27566              
==========================================
- Hits       105231   104237     -994     
- Misses       7583     8772    +1189     
+ Partials     1301     1106     -195     
Flag Coverage Δ
backend 94.13% <100.00%> (-1.55%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/queue/processors.ts 94.53% <100.00%> (ø)

... and 3 files with indirect coverage changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

perf: actuation lock is claimed after the refresh it should prevent — 1,180 discarded passes in 2h

1 participant